Skip to content

refactor: extract report command#153

Closed
ndycode wants to merge 4 commits intorefactor/pr1-check-commandfrom
refactor/pr1-report-command
Closed

refactor: extract report command#153
ndycode wants to merge 4 commits intorefactor/pr1-check-commandfrom
refactor/pr1-report-command

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 20, 2026

Summary

  • extract the codex auth report implementation into a dedicated command module
  • add direct unit coverage for the report command while keeping the existing CLI output behavior stable

What Changed

  • added lib/codex-manager/commands/report.ts with report argument parsing, output generation, and file-write handling
  • updated lib/codex-manager.ts so the report path delegates to the extracted command module
  • added test/codex-manager-report-command.test.ts for help, invalid-option, and JSON/file-output coverage

Validation

  • npm run lint
  • npm run typecheck
  • npm run test -- test/codex-manager-report-command.test.ts test/codex-manager-check-command.test.ts test/codex-manager-status-command.test.ts test/codex-manager-switch-command.test.ts test/codex-manager-cli.test.ts
  • npm run build

Risk and Rollback

  • Risk level: medium-low
  • Rollback plan: revert 8368528 to restore the inline report implementation

Additional Notes

  • this PR remains stacked on the command-split train from the same worktree

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

extracts the codex auth report implementation from the monolithic codex-manager.ts into lib/codex-manager/commands/report.ts, following the same command-split pattern used for check and status. the dep-injection approach (ReportCommandDeps) makes the new module fully unit-testable without touching the real filesystem or network.

key changes:

  • defaultWriteFile now uses a temp-file + atomic rename pattern with EBUSY/EPERM retry on the rename — resolving both prior review concerns (bare write and temp-file leak)
  • serializeForecastResults is duplicated between codex-manager.ts (still used by runForecast) and the new report.ts; a shared utility would be cleaner but is out of scope for this PR
  • RETRYABLE_WRITE_CODES covers EBUSY/EPERM but omits EAGAIN, which is inconsistent with the unified-settings.ts convention
  • defaultWriteFile retry logic has no vitest coverage — all tests inject a mock writeFile, leaving the windows-safety path unexercised

Confidence Score: 4/5

  • safe to merge; prior blocking concerns are resolved and remaining items are non-blocking follow-ups
  • both previous P1 concerns (bare fs.writeFile, temp-file leak) are fully addressed. remaining gaps are a missing EAGAIN code and untested defaultWriteFile retry path — real but not production-blocking. the delegation refactor in codex-manager.ts is clean and the new dep-injection design is solid.
  • lib/codex-manager/commands/report.ts — EAGAIN gap and untested defaultWriteFile retry; test/codex-manager-report-command.test.ts — defaultWriteFile path never exercised

Important Files Changed

Filename Overview
lib/codex-manager/commands/report.ts new command module; temp-write + rename pattern addresses prior review concerns; EAGAIN missing from RETRYABLE_WRITE_CODES vs project convention; serializeForecastResults is duplicated from codex-manager.ts
lib/codex-manager.ts clean delegation to runReportCommand with all required deps; unused dirname/resolve imports correctly removed; no issues
test/codex-manager-report-command.test.ts good coverage of help, invalid options, live probe branches, null storage, and injected write failures; defaultWriteFile retry path untested because writeFile is always injected

Sequence Diagram

sequenceDiagram
    participant CLI as codex-manager.ts
    participant CMD as runReportCommand
    participant DEPS as injected deps
    participant FS as defaultWriteFile

    CLI->>CMD: runReportCommand(rest, deps)
    CMD->>DEPS: setStoragePath(null)
    CMD->>DEPS: getStoragePath()
    CMD->>DEPS: loadAccounts()
    opt --live flag
        loop each enabled account
            CMD->>DEPS: queuedRefresh(refreshToken)
            CMD->>DEPS: fetchCodexQuotaSnapshot(...)
        end
    end
    CMD->>CMD: evaluateForecastAccounts(...)
    CMD->>CMD: build report object
    opt --out flag
        alt writeFile dep provided
            CMD->>DEPS: writeFile(outputPath, json)
        else default
            CMD->>FS: defaultWriteFile(outputPath, json)
            FS->>FS: fs.mkdir(dirname)
            FS->>FS: fs.writeFile(tempPath)
            loop up to 5 rename attempts (EBUSY/EPERM retry)
                FS->>FS: fs.rename(tempPath → path)
            end
        end
    end
    CMD-->>CLI: exit code (0 or 1)
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/codex-manager/commands/report.ts
Line: 29

Comment:
**`EAGAIN` missing from retry set**

`RETRYABLE_WRITE_CODES` only covers `EBUSY` and `EPERM`, but `unified-settings.ts` retries on `EBUSY/EPERM/EAGAIN` — the project's established convention per `lib/AGENTS.md`. `EAGAIN` can surface on linux network-mounted paths and some windows scenarios; omitting it makes `defaultWriteFile` inconsistent with the rest of the codebase.

```suggestion
const RETRYABLE_WRITE_CODES = new Set(["EBUSY", "EPERM", "EAGAIN"]);
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/codex-manager-report-command.test.ts
Line: 240-255

Comment:
**`defaultWriteFile` rename-retry path has no coverage**

every test injects `writeFile` as a dep, so `defaultWriteFile` is never exercised. the EBUSY/EPERM retry-on-rename logic (the core windows-safety mechanism) is completely untested — consistent with the test pattern in `unified-settings.test.ts` which has explicit tests for EBUSY/EPERM retries and temp-file cleanup.

consider adding a test that calls `runReportCommand` **without** a `writeFile` override (so `defaultWriteFile` runs) with a mocked `fs` module, or extract `defaultWriteFile` into a separate testable utility. at minimum, a test that verifies temp-file cleanup on a failing rename would close the most important gap from the previous review thread.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (4): Last reviewed commit: "test: harden report command path asserti..." | Re-trigger Greptile

Context used:

  • Context used - lib/AGENTS.md (source)
  • Context used - test/AGENTS.md (source)

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 40443e99-cd5e-4309-a740-1a9406969508

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr1-report-command
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr1-report-command

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode ndycode added the passed label Mar 22, 2026
@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant